-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(integ-runner): support custom --app
commands
#22761
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @karakter98 Apologies for the delay in reviewing this.
Just got a minor comment and would like a clarification around the single quote thing.
@@ -68,7 +68,8 @@ to be a self contained CDK app. The runner will execute the following for each f | |||
Read the list of tests from this file | |||
- `--disable-update-workflow` (default=`false`) | |||
If this is set to `true` then the [update workflow](#update-workflow) will be disabled | |||
|
|||
- `--app` | |||
The custom CLI command that will be used to run the test files. You can include {filePath} to specify where in the command the test file name should be inserted. Example: --app='"python3.8 {filePath}"'. It is recommended to use single quotes to wrap the double quotes of the command into a literal string, to avoid parsing errors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
file path instead of file name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is recommended to use single quotes to wrap the double quotes of the command into a literal string, to avoid parsing errors.
Could you elaborate on this recommendation?
--app
commands
✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.
@karakter98 I've applied my suggestions and approved to PR to move this forward. Let me know if you can clarify this recommendation. Very happy to add it back in if it makes sense.
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
… files (#22786) Follow-up to #22761. To support other languages than JavaScript (see #22521) we need to be able to detect test files with any patterns. With this PR, users can specify a number of custom `--test-regex` patterns that will bed used to discover integration test files. Together with `--app` this can already be used to run integ tests in arbitrary languages. Example usage: `integ-runner --app="python3 {filePath}" --test-regex="^integ_.*\.py$"` Also contains a minor refactor to make `--app` available via `IntegrationTests.fromFile()`. This is in preparation of an upcoming change to reestablish support for an integration test config file. ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@mrgrain sorry for the delay, the recommendation was put there because of limitations in the This comment explains the problem well, if you need to add flags to the app command string (like |
Thanks for the clarification. I cannot reproduce the problem on my end, plus the linked yargs issues has been closed as completed since 2019. It's also not a recommendation we make elsewhere. Let's monitor, but I'm inclined to keep it out for now (like it has been merged) unless we get an influx of issues. |
To prepare for supporting other languages than TypeScript (see #22521) we need to be able to run test files with commands other than the current default
node
. With this PR, users can specify a custom--app
command that will substitute the string{filePath}
with the filename of each discovered test, and synth each test with that command.Example usage:
integ-runner --app '"node --no-warnings {filePath}"'
Until we also allow discovery of different file prefixes, this can't be used to run languages like Python, because the default prefix used now (
integ.
) contains a.
which is not a valid character for Python module names.All Submissions:
Adding new Unconventional Dependencies:
New Features
yarn integ
to deploy the infrastructure and generate the snapshot (i.e.yarn integ
without--dry-run
)?By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license